New doc on how to integrate an operator #377

rajatchopra · 2018-09-28T23:23:08Z

A document to help an operator developer to integrate the operator with the installer. It covers what files to keep where so that configurations can be properly generated.

abhinavdahiya · 2018-09-29T00:04:15Z

docs/dev/integrate_operator_how_to.md

This is not recommended. Only special operators require this.
cc @smarterclayton

The files should be <internal order>_<name>.yaml like here https://github.com/openshift/machine-config-operator/tree/master/install

00-namaspace.yaml ... 03-roles.yaml 04-serviceaccount.yaml 05-deplopment.yaml

abhinavdahiya · 2018-09-29T00:07:20Z

docs/dev/integrate_operator_how_to.md

Upshot of the above document is that the operator should keep its static manifest files in manifests/ folder of the operator repository and manage image references through an image-references file.

The document above requires more than keeping in /manifests of the repo. I would recommend not duplicate here.

The order of creation of the files is alphabetical, so the files should be numbered like below to effectively create the service account before the deployment.

ClusterVersionOperator applies the manifests to cluster in alphabetical order. So it is recommended that manifests be order of dependencies.
For example,

a deployment requires service account, roles etc and,

everything requires the namespace to exist.

So the order of the manifests should be enforces like:

The document above requires more than keeping in /manifests of the repo. I would recommend not duplicate here.

Agreed on not duplicating here. And any references to this that we keep should talk about the operator images; the operator Git repository doesn't come into it.

But if we punt this portion of the docs to external docs (whether in a Google Doc or github.com/openshift/cluster-version-operator Markdown), what's left to talk about here? Just:

Almost everyone should use external docs. {details about when that won't work}. {where to put things when you add them to this repo}.

abhinavdahiya · 2018-09-29T00:12:39Z

docs/dev/integrate_operator_how_to.md

00-namaspace.yaml ... 03-roles.yaml 04-serviceaccount.yaml 05-deplopment.yaml

abhinavdahiya · 2018-09-29T00:13:10Z

docs/dev/integrate_operator_how_to.md

drop this. not required for most operators other than internal ordering.

abhinavdahiya · 2018-09-29T00:14:30Z

docs/dev/integrate_operator_how_to.md

either keep it in memory as users shouldn't be editing it.

push the discovered config to api, as users might change this in future.

@abhinavdahiya do you have an example of 'push the discovered config to api, as users might change this in future'?

openshift-ci-robot · 2018-09-29T02:25:38Z

@rajatchopra: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	4f66f9763ee71ea9cbabc3f0dc1edf8757f29727	link	`/test e2e-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

wking

A few nits inline, take 'em if they sound useful ;).

More broadly, I'm not clear on how this is scoped vs. the Google Doc, and I feel like the CVO repository is feeling lonely and left-out. @smarterclayton, can you provide more details about where you see these docs being maintained long-term? If it's in multiple locations, I think we need clear scoping about what goes where so we can give good cross-links to guide newcomers between locations.

wking · 2018-09-29T22:40:25Z

docs/dev/integrate_operator_how_to.md

nit: The page title should be h1 (one #), and subsections should increment by one level. It's not our fault if we think GitHub styles them too big ;).

Also, do we expect other levels of operator docs besides this? It seems unlikely to me, in which case I'd prefer docs/dev/operators.md and

# Operator integration

h1 is indeed to big for a smallish document like this one. Too bad if someone is forced into a telescopic lens. Can I stick with h2/h4?

No. Headings should not be used for styling. That's what CSS is for. Just stick to the standard hierarchy.

Ack.
Convinced? No.
Complied? Yes.

docs/dev/integrate_operator_how_to.md

wking · 2018-09-29T23:13:19Z

docs/dev/integrate_operator_how_to.md

The document above requires more than keeping in /manifests of the repo. I would recommend not duplicate here.

Agreed on not duplicating here. And any references to this that we keep should talk about the operator images; the operator Git repository doesn't come into it.

But if we punt this portion of the docs to external docs (whether in a Google Doc or github.com/openshift/cluster-version-operator Markdown), what's left to talk about here? Just:

Almost everyone should use external docs. {details about when that won't work}. {where to put things when you add them to this repo}.

wking · 2018-09-29T23:24:25Z

docs/dev/integrate_operator_how_to.md

I think it's worth spending a bit more time (wherever these "standard approach" docs live), talking about how this wires in. As I understand it, you'd take the /manifests route with enough static content to get your operator launched, and then you pull everything else you need (e.g. the install config) live from well-known locations. You focus on the last part of that here, and the Google Doc focuses on the first part. I don't see anywhere that's tying them together, although the MCO link you have below probably gives a good example for folks who want to read the source ;).

I have done a bit of a shift up to tie the two methods (one for static, the other for templates). Review again please.

I have done a bit of a shift up to tie the two methods (one for static, the other for templates).

This paragraph looks largely unchanged from 4f66f97 -> 2650b11. I was thinking of an outline more like:

The recommended way

...Everyone following this integration path will need static manifests/configs (as discussed in the Google Doc) to launch their operator...

Dynamic configuration

Some operators will need dynamic configuration to accomplish their task. These operators should set up their static manifests/configs to launch their operator, and then have their operator fill in its configuration by pulling information from cluster resources. For example, you can pull the install-config as a config map:

...

docs/dev/integrate_operator_how_to.md

wking · 2018-09-29T23:28:19Z

docs/dev/integrate_operator_how_to.md

nit: "templateized files" -> "templates"?

And including the paths and variable names here seems brittle. But it's probably fine until we have commits we can link to showing good examples of adding a new operator so folks can pattern-match.

Fixed.
Agree on the paths and variable names. Will be intractable at some point.

docs/dev/integrate_operator_how_to.md

abhinavdahiya · 2018-10-03T18:57:46Z

/approve

He's approved this PR since: #377 (comment)

wking · 2018-10-03T20:19:00Z

Can this be docs/dev/operators.md? Also, is the Google Doc public?

rajatchopra · 2018-10-03T23:09:36Z

Can this be docs/dev/operators.md? Also, is the Google Doc public?

Renamed the file.
The doc is not public, but I guess the answer is have a PR in CVO repo with the contents of those docs.

wking · 2018-10-03T23:29:18Z

The doc is not public, but I guess the answer is have a PR in CVO repo with the contents of those docs

@smarterclayton, would that be ok with you? If not, do we have a plan for public integration docs for "The recommended way"?

rajatchopra · 2018-10-04T20:50:07Z

The doc is not public, but I guess the answer is have a PR in CVO repo with the contents of those docs

@smarterclayton, would that be ok with you? If not, do we have a plan for public integration docs for "The recommended way"?

Quick link to the doc in question here: https://docs.google.com/document/d/1CGZVEyuloZ9oD4NUArW6dnEpi0WFc6BP2tqPSwFZTCY/

smarterclayton · 2018-10-04T22:59:11Z

We should probably scrub the doc, get any final short term feedback in, and then make public. Alternatively we can just grab the “how do I” segments out and put them into docs directly. On Oct 4, 2018, at 4:50 PM, Rajat Chopra <[email protected]> wrote: The doc is not public, but I guess the answer is have a PR in CVO repo with the contents of those docs @smarterclayton <https://github.com/smarterclayton>, would that be ok with you? If not, do we have a plan for public integration docs for "The recommended way"? Quick link to the doc in question here: https://docs.google.com/document/d/1CGZVEyuloZ9oD4NUArW6dnEpi0WFc6BP2tqPSwFZTCY/ — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#377 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pzpIZHCo7rohyrO16NIrLBYquZFZks5uhnSNgaJpZM4XAMmo> .

rajatchopra · 2018-10-08T21:52:22Z

@wking PTAL: openshift/cluster-version-operator#34

steveej

Questions and language nits in the comments. Another one would be to use render instead of fill up to describe the temple process.

steveej · 2018-10-09T14:04:27Z

docs/dev/operators.md

Maybe use a ### headline here?

steveej · 2018-10-09T14:05:26Z

docs/dev/operators.md

Maybe use a ### headline here?

steveej · 2018-10-09T14:06:40Z

docs/dev/operators.md

steveej · 2018-10-09T14:08:43Z

docs/dev/operators.md

s/operators/operator's/

steveej · 2018-10-09T14:09:17Z

docs/dev/operators.md

s/example/an example/

steveej · 2018-10-09T14:13:58Z

docs/dev/operators.md

What is the actual choice here in the last sentence, i.e. what's the alternative to "vendor the operator’s github pkg and return the entire list of configs and manifests in the Generate function"?

karmab · 2018-10-09T20:00:32Z

docs/dev/operators.md

namaste or namespace ?

wking · 2018-10-10T02:16:48Z

docs/dev/operators.md

You can replace this reference, and possibly the whole recommended-way section, with a link to here now that openshift/cluster-version-operator#34 has landed.

This document is meant for an operator author, who wants to integrate a second-level operator into the installer. The document is a guide on all the possible and acceptable methods.

abhinavdahiya · 2018-10-10T23:10:33Z

/lgtm

openshift-ci-robot · 2018-10-10T23:10:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, rajatchopra

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya,rajatchopra]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This content is descended from openshift/installer@9c8a77dd (docs/dev: Doc on how to integrate an operator, 2018-09-28, openshift/installer#377), but handling dynamic configuration is a generic issue, so the docs should live in the generic CVO repository. This will make these docs more discoverable. It will also allow the installer docs to focus on installer-specific issues for the small subset of operators that need direct installer bindings.

openshift-ci-robot requested review from staebler and steveej September 28, 2018 23:23

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 28, 2018

abhinavdahiya suggested changes Sep 29, 2018

View reviewed changes

abhinavdahiya previously requested changes Sep 29, 2018

View reviewed changes

wking reviewed Sep 29, 2018

View reviewed changes

pecameron reviewed Oct 1, 2018

View reviewed changes

docs/dev/integrate_operator_how_to.md Outdated Show resolved Hide resolved

rajatchopra force-pushed the patch-1 branch 5 times, most recently from 2c6f306 to 2650b11 Compare October 3, 2018 00:05

wking reviewed Oct 3, 2018

View reviewed changes

docs/dev/integrate_operator_how_to.md Outdated Show resolved Hide resolved

rajatchopra force-pushed the patch-1 branch from 2650b11 to 92bab36 Compare October 3, 2018 16:38

rajatchopra force-pushed the patch-1 branch from 92bab36 to 8252efd Compare October 3, 2018 23:08

rajatchopra mentioned this pull request Oct 8, 2018

docs/dev: Second level operator FAQ openshift/cluster-version-operator#34

Merged

steveej reviewed Oct 9, 2018

View reviewed changes

karmab reviewed Oct 9, 2018

View reviewed changes

docs/dev/operators.md Outdated

Copy link

Contributor

karmab Oct 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

namaste or namespace ?

wking reacted with laugh emoji

wking reviewed Oct 10, 2018

View reviewed changes

rajatchopra force-pushed the patch-1 branch from 8252efd to d7720f9 Compare October 10, 2018 22:59

rajatchopra force-pushed the patch-1 branch 2 times, most recently from c734231 to 4f8e7ac Compare October 10, 2018 23:02

docs/dev: Doc on how to integrate an operator

9c8a77d

This document is meant for an operator author, who wants to integrate a second-level operator into the installer. The document is a guide on all the possible and acceptable methods.

rajatchopra force-pushed the patch-1 branch from 4f8e7ac to 9c8a77d Compare October 10, 2018 23:07

openshift-ci-robot assigned abhinavdahiya Oct 10, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 10, 2018

openshift-merge-robot merged commit 42f7541 into openshift:master Oct 10, 2018

rajatchopra deleted the patch-1 branch October 11, 2018 20:11

This was referenced Nov 27, 2018

docs/dev/operators: Document a dynamic-object approach openshift/cluster-version-operator#59

Closed

docs/dev/operators: Drop docs for the CVO approach #733

Merged

New doc on how to integrate an operator #377

New doc on how to integrate an operator #377

Uh oh!

Conversation

rajatchopra commented Sep 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Sep 29, 2018

Uh oh!

wking left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking Sep 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

The recommended way

Dynamic configuration

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

abhinavdahiya commented Oct 3, 2018

Uh oh!

wking commented Oct 3, 2018

Uh oh!

rajatchopra commented Oct 3, 2018

Uh oh!

wking commented Oct 3, 2018

Uh oh!

rajatchopra commented Oct 4, 2018

Uh oh!

smarterclayton commented Oct 4, 2018 via email

Uh oh!

rajatchopra commented Oct 8, 2018

Uh oh!

steveej left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rajatchopra commented Sep 28, 2018 •

edited

Loading

wking Sep 29, 2018 •

edited

Loading